Summary

This PR adds NUnit Platform attribute conversion to RunOn/ExcludeOn and XUnit Record.Exception conversion to try-catch blocks for the migration code fixers.

Critical Issues

None found ✅

Suggestions

1. NUnit Platform Attribute - Positional Argument Bug

In NUnitMigrationCodeFixProvider.cs:ConvertPlatformAttribute (line 222-228), the positional argument handling has a logic issue:

if (argName == "Include" || argName == null)
{
    // Named argument Include= or positional argument (which is Include)
    includeValue = value;
}

Issue: If there are multiple positional arguments (e.g., [Platform("Win", "Linux")]), the code will overwrite includeValue for each one, only keeping the last value. NUnit's Platform attribute doesn't actually support multiple positional arguments in this way, but the code should be more defensive.

Suggestion: Consider only processing the first positional argument or documenting this behavior.

2. Record.Exception - Missing Block Body Handling

In XUnitMigrationCodeFixProvider.cs:ConvertLambdaBodyToStatement (line 257-265), when a lambda has a block body with multiple statements:

if (body is BlockSyntax block)
{
    // If it's a single statement block, extract the statement
    if (block.Statements.Count == 1)
    {
        return block.Statements[0];
    }
    // Otherwise return the block as-is (will need to be a compound statement)
    return block;
}

Issue: When returning the block directly for multi-statement lambdas, it's placed inside the try block as a nested block:

try
{
    {  // Extra braces here
        statement1;
        statement2;
    }
}

Suggestion: When actionBody is already a BlockSyntax with multiple statements in CreateTryCatchStatements, extract its statements instead of nesting it:

if (actionBody is BlockSyntax blockBody && blockBody.Statements.Count > 1)
{
    // Use block's statements directly
    tryBodyStatement = SyntaxFactory.Block(blockBody.Statements)...
}

3. Record.ExceptionAsync - Missing Async/Await Verification

In XUnitMigrationCodeFixProvider.cs:CreateTryCatchStatements (line 286-298), the async handling only checks the invocation name but doesn't verify if the action body actually returns a Task:

// If async, we may need to await the action
if (isAsync && tryBodyStatement is ExpressionStatementSyntax exprStmt)
{
    var awaitExpr = SyntaxFactory.AwaitExpression(...);

Issue: This blindly adds await for any Record.ExceptionAsync usage, even if the lambda body is synchronous (e.g., () => throw new Exception()). While XUnit's Record.ExceptionAsync does expect async delegates, users might have incorrect code that still compiles.

Suggestion: Consider using semantic analysis to check if the expression is actually awaitable, or document that this migration assumes correct XUnit usage.

4. Platform Attribute - Case Sensitivity

In NUnitMigrationCodeFixProvider.cs:MapNUnitPlatformToTUnitOS (line 274), the mapping uses ToLowerInvariant():

return nunitPlatform.ToLowerInvariant() switch
{
    "win" or "win32" or ... => "Windows",

Minor: NUnit's platform matching is case-insensitive, so this is correct. However, consider adding a comment explaining this matches NUnit's behavior, as it's not immediately obvious why we're lower-casing.

5. Test Coverage - Missing Edge Cases

The tests cover basic scenarios well, but consider adding tests for:

• [Platform(Include = "UnknownPlatform")] - what happens with unknown platforms?
• [Platform(Include = "Win", Exclude = "Linux")] - both Include and Exclude (currently only Include is used)
• Record.Exception with method groups: Record.Exception(ThrowingMethod) instead of lambdas
• Record.ExceptionAsync with async lambdas: async () => await SomeAsyncMethod()

Verdict

✅ APPROVE - No critical issues

The implementation is solid and follows TUnit conventions well. The suggestions above are optional improvements for edge cases and code robustness, but not blocking issues.